fix(rome_js_formatter): print empty statements as empty blocks #2504

ematipico · 2022-04-27T14:28:10Z

Summary

Test Plan

Added new cases, one for the if and one for do while

github-actions · 2022-04-27T14:30:53Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12391	12391	0
Failed	3866	3866	0
Panics	0	0	0
Coverage	76.22%	76.22%	0.00%

github-actions · 2022-04-27T14:33:11Z

Playground for commit b8c694d

cloudflare-workers-and-pages · 2022-04-27T14:34:04Z

Deploying with Cloudflare Pages

Latest commit:	`c7aa984`
Status:	✅ Deploy successful!
Preview URL:	https://641e0373.tools-8rn.pages.dev

View logs

NicholasLYang · 2022-04-27T15:12:13Z

crates/rome_js_formatter/src/js/statements/empty_statement.rs


 impl FormatNode for JsEmptyStatement {
    fn format_fields(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
        let JsEmptyStatementFields { semicolon_token } = self.as_fields();
-
-        Ok(formatter.format_replaced(&semicolon_token?, empty_element()))
+        let parent_kind = self.syntax().parent().map(|p| p.kind());


Nit: this pattern shows up a couple times. Should we have a function that gets the parent kind directly?

leops

When it comes to unifying the printing behavior of empty statements, I would suggest we go the opposite way and extend the behavior of if statements (always insert a block) to all other affected statements (for loops, for in loops, for of loops, while loops, do while loops and with blocks).
The reasoning behind this is that Rome JS had a useBlockStatements lint rule that emitted a fixable warning for these nodes if they were using a single statement as their body, but for this kind of "style fix" we might as well rewrite it directly in the formatter.

ematipico · 2022-04-27T15:24:04Z

When it comes to unifying the printing behavior of empty statements, I would suggest we go the opposite way and extend the behavior of if statements (always insert a block) to all other affected statements (for loops, for in loops, for of loops, while loops, do while loops and with blocks). The reasoning behind this is that Rome JS had a useBlockStatements lint rule that emitted a fixable warning for these nodes if they were using a single statement as their body, but for this kind of "style fix" we might as well rewrite it directly in the formatter.

Does that mean that we want to intentionally diverge from prettier?

leops · 2022-04-27T15:49:01Z

Does that mean that we want to intentionally diverge from prettier?

Well at least I think we should discuss whether we want to have code-style related lint rules in the analyzer or to enforce these decisions in the formatter directly. Ultimately the decision is probably about how disruptive the change is (how often are these single-statement syntax actually used) vs. how confusing it is for users to see fixable diagnostics in their code for things they'd expect the formatter to take care of.

ematipico · 2022-04-27T16:07:52Z

Does that mean that we want to intentionally diverge from prettier?

Well at least I think we should discuss whether we want to have code-style related lint rules in the analyzer or to enforce these decisions in the formatter directly. Ultimately the decision is probably about how disruptive the change is (how often are these single-statement syntax actually used) vs. how confusing it is for users to see fixable diagnostics in their code for things they'd expect the formatter to take care of.

I completely forgot about that lint rule. Yes, in Rome classic there was this idea where "coding style rules" should not exists because we own the formatter too.

This is the rule FYI: https://github.com/rome/tools/blob/archived-js/internal/compiler/lint/rules/js/useBlockStatements.test.md

At this point I think we should just diverge from prettier. I agree with you @leops

yassere · 2022-04-27T16:48:03Z

At this point I think we should just diverge from prettier. I agree with you @leops

I don't think we should. This is a style decision, and we should have a very high bar for where we diverge from Prettier purely for style reasons (rather than implementation complexity).

We don't necessarily need to add every lint rule that existed in Rome classic. But if we do end up with some "code style rules", I think that's ok because we've intentionally relinquished some of our control over the formatting output by targeting Prettier equivalence.

ematipico · 2022-04-27T19:08:15Z

@yassere Could let us know why though? Why you think we should not put blocks and why we don't need that lint rule anymore.

I think it makes sense to have blocks. Having empty statements can lead to errors, same for expressions without blocks.

NicholasLYang · 2022-04-27T19:33:28Z

Personally I view diverging from Prettier as a large enough reason. If we get far enough in linting that we create this rule again, we can consider diverging then. But until that time, we should do what's best for the formatter, and not what might be best for the linter. IMO, as long as we're not seriously inhibiting future tools, we should do what's best for the tools that currently exist. Each of our tools needs to gain adoption and the promise of a linter feature does not outweigh breaking prettier compatibility, at least for me.

yassere

@ematipico I think it makes sense to have blocks too. Maybe that's something we can have as a safe autofix for the linter. But for the formatter, I think we need to respect our commitment to match Prettier's styling decisions as much as possible, unless we have a very good reason not to.

Given our previous messaging, intentionally deviating from Prettier's style is not a decision we should make lightly. Each case should be discussed in its own issue so that we can make a strong case to justify that divergence and document it.

ematipico temporarily deployed to aws April 27, 2022 14:28 Inactive

ematipico marked this pull request as draft April 27, 2022 14:31

ematipico force-pushed the fix/do-while-empty-body branch from 358eec3 to 802baa2 Compare April 27, 2022 14:34

ematipico marked this pull request as ready for review April 27, 2022 14:34

ematipico temporarily deployed to aws April 27, 2022 14:34 Inactive

fix(rome_js_formatter): print empty statements differently

f02529d

ematipico force-pushed the fix/do-while-empty-body branch from 802baa2 to f02529d Compare April 27, 2022 14:34

ematipico temporarily deployed to aws April 27, 2022 14:34 Inactive

ematipico requested review from leops, NicholasLYang and yassere April 27, 2022 14:35

NicholasLYang reviewed Apr 27, 2022

View reviewed changes

NicholasLYang approved these changes Apr 27, 2022

View reviewed changes

leops reviewed Apr 27, 2022

View reviewed changes

ematipico temporarily deployed to aws April 27, 2022 16:19 Inactive

chore: add empty block on empty statements

c7aa984

ematipico force-pushed the fix/do-while-empty-body branch from 333a6e2 to c7aa984 Compare April 27, 2022 16:19

ematipico temporarily deployed to aws April 27, 2022 16:19 Inactive

ematipico changed the title ~~fix(rome_js_formatter): print empty statements differently~~ fix(rome_js_formatter): print empty as empty blocks Apr 27, 2022

ematipico changed the title ~~fix(rome_js_formatter): print empty as empty blocks~~ fix(rome_js_formatter): print empty statements as empty blocks Apr 27, 2022

yassere reviewed Apr 27, 2022

View reviewed changes

ematipico closed this Apr 27, 2022

rome locked and limited conversation to collaborators Apr 27, 2022

rome unlocked this conversation Apr 27, 2022

ematipico mentioned this pull request Apr 27, 2022

🐛 loop/if with empty statement body #2448

Closed

1 task

ematipico deleted the fix/do-while-empty-body branch May 26, 2022 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rome_js_formatter): print empty statements as empty blocks #2504

fix(rome_js_formatter): print empty statements as empty blocks #2504

ematipico commented Apr 27, 2022

github-actions bot commented Apr 27, 2022

github-actions bot commented Apr 27, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 27, 2022 •

edited

Loading

NicholasLYang Apr 27, 2022

leops left a comment

ematipico commented Apr 27, 2022

leops commented Apr 27, 2022

ematipico commented Apr 27, 2022

yassere commented Apr 27, 2022

ematipico commented Apr 27, 2022

NicholasLYang commented Apr 27, 2022

yassere left a comment

fix(rome_js_formatter): print empty statements as empty blocks #2504

fix(rome_js_formatter): print empty statements as empty blocks #2504

Conversation

ematipico commented Apr 27, 2022

Summary

Test Plan

github-actions bot commented Apr 27, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

ts/babel

ts/microsoft

github-actions bot commented Apr 27, 2022 • edited Loading

cloudflare-workers-and-pages bot commented Apr 27, 2022 • edited Loading

Deploying with Cloudflare Pages

NicholasLYang Apr 27, 2022

Choose a reason for hiding this comment

leops left a comment

Choose a reason for hiding this comment

ematipico commented Apr 27, 2022

leops commented Apr 27, 2022

ematipico commented Apr 27, 2022

yassere commented Apr 27, 2022

ematipico commented Apr 27, 2022

NicholasLYang commented Apr 27, 2022

yassere left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 27, 2022 •

edited

Loading